Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: initial implementation #1

Merged
merged 61 commits into from
Oct 30, 2024
Merged

feat: initial implementation #1

merged 61 commits into from
Oct 30, 2024

Conversation

aschmahmann
Copy link
Contributor

This is an initial implementation of the DNS server using CoreDNS.

The main missing feature for getting this to work is integration with a distributed database to be shared between the primary and secondary nameservers. It might be possible to do this using zone files as well, but using something like dynamodb for now seems fine.

@aschmahmann aschmahmann force-pushed the feat/initial-implementation branch 3 times, most recently from 5928892 to 966722d Compare August 16, 2024 22:49
README.md Outdated
To set an ACME challenge send an HTTP request to the server (for libp2p.direct this is registration.libp2p.direct)
```shell
curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
-H "Authorization: Bearer <signature>.<public_key>"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on why JWT did not feel great because

  • It takes the entire payload and puts it in the authorization header
    • No standards around body hash, including public keys, etc.
  • IIUC most implementations don't seem to support secp256k1 and IMO we'd want to enable each of the 4 currently supported key types (RSA, Ed25519, ECDSA, Secp256k1)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to follow RFC 6750? The "Bearer" Auth scheme is registered to that RFC https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml.

I haven't looked closer here, because I'm assuming we'll opt to use the HTTP Peer ID authentication flow instead.

README.md Outdated
Comment on lines 60 to 61
curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
-H "Authorization: Bearer <signature>.<public_key>"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: We could've avoided doing anything special with the auth if we'd relied on libp2p doing the auth for us.

About the protocol format:

  • custom wire protocol over libp2p seemed like overkill
  • HTTP over libp2p would be nice and use the exact same semantics but without the auth header

About using libp2p for the transport:

  • The initial connection establishment and loadbalancing stories don't seem as easy as we'd want at the moment
    • I don't want to have to embed a peerID into deployed applications (e.g. what if we have to rotate keys) and while we can get the other approaches working they feel not out of the box yet
      • Could use CA certs instead of peerIDs for the outbound connection, but that requires more new code in the libp2p implementations
      • We could use DNSAddr (e.g. /dnsaddr/registration.libp2p.direct without a peerID suffix) which seems fine (even more so with DNSSEC), although a bit of extra wrapper code likely needed in each implementation since IIRC it's not supported out of the box in most implementations

It'd be nice to evolve this into being able to do HTTP over libp2p and drop the auth header by doing one or both of:

  • Enable DNSAddr or CA based connection establishment
  • Land the HTTP PeerID Auth spec (yes it means two round-trips instead of one but that doesn't seem like a big deal)

The result means there'd be very little custom code here at all given the user is already leveraging a libp2p implementation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

custom wire protocol over libp2p seemed like overkill

Land the HTTP PeerID Auth spec (yes it means two round-trips instead of one but that doesn't seem like a big deal)

Both of these seem like desire-able things that we may want at some point in the future. Is it worth investigating now before we implement some other solution that quickly goes out of date?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

custom wire protocol over libp2p seemed like overkill

This doesn't seem particularly desirable to bother with.

Land the HTTP PeerID Auth spec

WDYT @MarcoPolo, does it make more sense to land the spec then do this business with the signed envelopes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The spec is very close to being ready. And if we have a use case, then I'd be very motivated to get it landed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we're essentially reusing the acme challenge as the authentication challenge. This is probably (?) okay for this use case, but I think it would better practice to not reuse challenges in this way. The HTTP Peer ID Auth flow would use a challenge just for the authentication (and that's where the extra round trip comes from).

README.md Outdated
}'
```

Where the signature is a base64 encoding of the signature for a [libp2p signed envelope](https://github.com/libp2p/specs/blob/master/RFC/0002-signed-envelopes.md)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed like grabbing this spec meant less overhead for implementers given that those using signed peer records (i.e. for gossipsub which is in most implementations) already do most of this. However, as can be seen from the Go code here go-libp2p hid most of the internals away such that actually getting at them was painful and/or required copy-pasting.

Alternatives welcome (including if it's just tossing all this and landing the HTTP PeerID Auth spec

README.md Outdated
Where the signature is a base64 encoding of the signature for a [libp2p signed envelope](https://github.com/libp2p/specs/blob/master/RFC/0002-signed-envelopes.md)
where:
- The domain separation string is "peer-forge-domain-challenge"
- The payload type is the ASCII string "/peer-forge-domain-challenge"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't planning on using a payload type and registering it in the codec table, however:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcoPolo @sukunrt might have thoughts on ^

acme/writer.go Outdated
return
}

const ttl = time.Hour
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea what the TTL should be here for these records given they shouldn't really be needed for very long. Thoughts?

Comment on lines +224 to +225
// TODO: Do we need to listen on the identify event instead?
// TODO: Where do we want to record this information if anywhere?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling these out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, and probably at least a log for now?

ForgeDomain string
}

const ttl = 1 * time.Hour
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it could be set to infinity. Any thoughts on the number, a day, 7 days, longer?

cmd/main.go Outdated
Comment on lines 25 to 27
func main() {
coremain.Run()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some details need to get hammered out here between this and the tests. We could also choose to use the stock Caddy binary and use everything we have as plugins. Whatever makes deployment and testing easier

Copy link

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just read the README.md and left some comments

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 60 to 61
curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
-H "Authorization: Bearer <signature>.<public_key>"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

custom wire protocol over libp2p seemed like overkill

Land the HTTP PeerID Auth spec (yes it means two round-trips instead of one but that doesn't seem like a big deal)

Both of these seem like desire-able things that we may want at some point in the future. Is it worth investigating now before we implement some other solution that quickly goes out of date?

README.md Outdated Show resolved Hide resolved
@aschmahmann aschmahmann force-pushed the feat/initial-implementation branch from 0cdda1a to 0ddd505 Compare August 20, 2024 20:03
README.md Outdated Show resolved Hide resolved
client/acme.go Outdated

var libp2pDirectWssComponent = multiaddr.StringCast("/tls/sni/*.libp2p.direct/ws")

func NewHostWithP2PForge(forgeDomain string, forgeRegistrationEndpoint string, opts ...libp2p.Option) (host.Host, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This function certainly, but most of this file is pretty WIP.

Will help to flesh this out with an end-to-end test that does:

  1. Sets up the DNS server
  2. Uses https://github.com/letsencrypt/pebble/ as a test ACME server that uses the DNS server
  3. Have a libp2p client (e.g. using this function or something similar) that gets a certificate for itself using the test ACME server + DNS server
  4. Have another libp2p client that can dial the first one using WSS with a custom client TLS config that allows the test CA cert

@aschmahmann aschmahmann force-pushed the feat/initial-implementation branch 3 times, most recently from 8f4af5e to 5653d6c Compare August 29, 2024 18:34
@aschmahmann aschmahmann force-pushed the feat/initial-implementation branch from 5653d6c to d1a0b6e Compare August 29, 2024 19:35
client/acme.go Outdated

var libp2pDirectWssComponent = multiaddr.StringCast("/tls/sni/*.libp2p.direct/ws")

func NewHostWithP2PForge(forgeDomain string, forgeRegistrationEndpoint string, caEndpoint string, userEmail string, trustedRoots *x509.CertPool, onCertLoaded func(), allowPrivateForgeAddrs bool, opts ...libp2p.Option) (host.Host, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constructor definitely is not great. Wanted to get something out before spending more time here, suggestions welcome.

Seems like this could go a whole lot easier if we didn't have a bunch of couplings going on:

  • Need TLS Config to instantiate the host (unless we cast to Swarm and add a transport later?), but need the host (and its addresses) to setup the certmagic TLS config
  • Trying to add a transport or an address would mess with the users' expectations of options passing, but we can't say "maybe add this address or transport if not already present"
  • We need to setup a listener (which requires knowing the DNS name), but the DNS name is determined by the address
    • We need to dynamically change the names as the IP addresses change

Could potentially change this to setting up a host, erroring if the websocket transport already exists, and then adding it + it's addresses + another address factory wrapper.

client/acme.go Outdated Show resolved Hide resolved
client/acme.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a first pass at the server side. I'll take a look at the client side next.

type acmeReader struct {
Next plugin.Handler
ForgeDomain string
Datastore datastore.Datastore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just define the interface we actually need in this package and not require a full Datastore implementation.

acme/reader.go Outdated
m.SetReply(r)
m.Authoritative = true
m.Answer = answers
w.WriteMsg(&m)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check err?

acme/writer.go Outdated
mux *mux.Router
}

func (c *acmeWriter) OnStartup() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this should be Start() error. I know it's passed into an OnStartup function to register as a callback, but the function itself is about starting the acmeWriter. There's also precedent in Go stdlib for Start(...) error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong feelings, I copied this from one of the CoreDNS examples https://github.com/coredns/coredns/blob/ee4d26b7807856efbf61ebebb2687df21561a787/plugin/health/setup.go#L22. If you want to change it feel free

acme/writer.go Outdated
c.nlSetup = true

c.mux.HandleFunc("/v1/{peerID}/_acme-challenge", func(w http.ResponseWriter, r *http.Request) {
authHeader := r.Header.Get("Authorization")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be so much better when we can use libp2p/go-libp2p#2854.

This could be something like:

authPeer := auth.ServerPeerIDAuth{
    // ...
    Next: func(id peer.ID, w http.ResponseWriter, r *http.Request) {
        err := c.updateDB(id, r.Body())
        // err handling
        w.WriteHeader(http.StatusOK)
    },
}
//...
c.mux.HandleFunc("/v1/_acme-challenge", authPeer)

Also is there a reason to have the peerID in the url?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, that's why I flagged it earlier in #1 (comment).

Do you think we're close enough to landing things that I should just drop and rewrite to use the server peerID auth now? Alternatively can just wait a bit and we'll delete some code before we launch.

Also is there a reason to have the peerID in the url?
We can remove it. It was mostly there to mimic some existing APIs for DNS puts that take domain names as arguments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s close. I just need a review from sukun and we’ll merge it. Very likely by next week.

acme/writer.go Outdated
return
}

// Value must be a base64url encoding of a SHA256 digest per https://datatracker.ietf.org/doc/html/rfc8555/#section-8.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should quote the relevant sentence verbatim "It MUST NOT contain any characters outside the base64url alphabet, including padding characters ("=")." As is, it's unclear if padding is allowed or not.

README.md Outdated
To set an ACME challenge send an HTTP request to the server (for libp2p.direct this is registration.libp2p.direct)
```shell
curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
-H "Authorization: Bearer <signature>.<public_key>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to follow RFC 6750? The "Bearer" Auth scheme is registered to that RFC https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml.

I haven't looked closer here, because I'm assuming we'll opt to use the HTTP Peer ID authentication flow instead.

client/challenge.go Outdated Show resolved Hide resolved
acme/writer.go Show resolved Hide resolved
Comment on lines +224 to +225
// TODO: Do we need to listen on the identify event instead?
// TODO: Where do we want to record this information if anywhere?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, and probably at least a log for now?

ipparser/plugin.go Outdated Show resolved Hide resolved
client/acme.go Outdated Show resolved Hide resolved
@aschmahmann aschmahmann force-pushed the feat/initial-implementation branch from 6f12d69 to e4350da Compare September 4, 2024 19:35
@aschmahmann aschmahmann force-pushed the feat/initial-implementation branch from e4350da to 308b83a Compare September 4, 2024 19:57
@aschmahmann aschmahmann force-pushed the feat/initial-implementation branch from 1f5b301 to eef1770 Compare September 16, 2024 20:10
aschmahmann and others added 18 commits September 16, 2024 16:29
glue records are fixed, bumping serial to see if it helps with
resolution at 8.8.8.8
glue records are ip4-only, this is just making sure we remove
any inconsistencies that could make google dns refuse resolution
this allows us to avoid hardcoding them in Kubo
registration one is commented for now, as we should measure failures as
well as success.

docs in docs/METRICS.md
This enables `coredns_forge_acme_registrations_total{status}`
which tracks number of request per response code, including ones
that failed libp2pPeerIDAuth

Also, switched to go-libp2p from main branch with http auth.
allows consumers like Kubo to adjust logger

NOTE: certmagic.DefaultACME.Logger and certmagic.Default.Logger
are used before we are able to set custom logger,
so consumer likely will want to set them to the same one.

We don't override upstream defaults in case application uses multiple
instances, and some of them are not related to p2p-forge.
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
_, _ = w.Write([]byte(fmt.Sprintf("error storing value: %s", err)))
return
}
w.WriteHeader(http.StatusOK)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be w.WriteHeader(http.StatusNoContent) to cause a 204 since we send no content.

Copy link
Contributor

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this, as the client shipped with Kubo 0.32.0-rc1, and we want to start producing docker images from main / staging.

Please fill separate issues / PRs for unaddressed issues.

@lidel lidel dismissed MarcoPolo’s stale review October 30, 2024 21:18

assiming outdated comments are addressed, if not, please fill them as separate issues

@lidel lidel merged commit c5971b0 into main Oct 30, 2024
4 checks passed
@lidel lidel deleted the feat/initial-implementation branch November 22, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants